-
Notifications
You must be signed in to change notification settings - Fork 1
Change mcp_servers map to single mcp_server configuration object in rules only #164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
|
@copilot No. Sorry. Rules contain MCP servers. These are just like tools. In the front-matter, the must specific one instance of the MCP server configuration. mcp_server:
command: python
args: ["-m", "server"]
env:
PYTHON_PATH: /usr/bin/python3
# Arbitrary nested fields below
custom_config:
host: localhost
port: 5432
ssl: true
pool:
min: 2
max: 10 |
Convert from map of server configs to single MCPServerConfig object. Each task/rule now specifies one MCP server configuration with standard fields (command, args, env, type, url, headers) plus arbitrary custom fields via the Content map. Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
Updated in commit 64b7225. Changed from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the MCP (Model Context Protocol) server configuration from a map-based approach to a single-object configuration model. Instead of each task/rule specifying multiple named MCP servers via a mcp_servers map, each task/rule now specifies one MCP server configuration via the mcp_server field. The Result.MCPServers() method now returns a slice of all MCP server configurations collected from rules and the task.
Key changes:
- API change:
mcp_serversmap field →mcp_serversingle object field in task/rule frontmatter - Return type change:
MCPServers()returns[]MCPServerConfigslice instead ofMCPServerConfigsmap - Documentation and examples updated to reflect the new single-server-per-file model
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/codingcontext/task_frontmatter.go | Changed MCPServers field from map to single MCPServer object |
| pkg/codingcontext/rule_frontmatter.go | Changed MCPServers field from map to single MCPServer object |
| pkg/codingcontext/result.go | Refactored MCPServers() to return slice and append configs from rules and task |
| pkg/codingcontext/task_frontmatter_test.go | Updated tests to use single MCP server object with Args field |
| pkg/codingcontext/rule_frontmatter_test.go | Updated tests to use single MCP server object with Args field |
| pkg/codingcontext/result_test.go | Rewrote tests for slice-based return type and removed override test case |
| pkg/codingcontext/README.md | Updated API documentation for new slice return type |
| docs/reference/file-formats.md | Updated reference docs with single-object configuration format and examples |
| examples/agents/tasks/example-with-standard-fields.md | Updated example to show single MCP server configuration |
| examples/agents/tasks/example-mcp-arbitrary-fields.md | Simplified example to show one server with arbitrary custom fields |
Remove the empty server detection logic from MCPServers() method as requested. The agent will handle filtering empty configs. Updated tests to expect empty MCPServerConfig structs in the returned slice. Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
Tasks no longer have mcp_server field. MCP servers are now only specified in rules. Result.MCPServers() now only returns configs from rules. Updated tests, examples, and documentation. Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
|
@copilot please resolve the conflicts. |
|
@copilot you have not resolved the conflicts. Make sure you fetch from master when you do this. Then |
Fetched from main and merged to resolve conflicts. All conflicts resolved: - Removed old task_frontmatter.go and rule_frontmatter.go files - Applied changes to new pkg/codingcontext/markdown/frontmatter.go structure - Removed MCPServer field from TaskFrontMatter - Changed RuleFrontMatter.MCPServers to MCPServer (single config) - Updated Result.MCPServers() to return []mcp.MCPServerConfig from rules only - Updated all tests to match new structure - All tests pass, linter passes Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
pkg/codingcontext/result.go
Outdated
| // Add server from each rule | ||
| for _, rule := range r.Rules { | ||
| if rule.FrontMatter.MCPServers != nil { | ||
| maps.Copy(servers, rule.FrontMatter.MCPServers) | ||
| } | ||
| } | ||
|
|
||
| // Add servers from task (overriding any from rules) | ||
| if r.Task.FrontMatter.MCPServers != nil { | ||
| maps.Copy(servers, r.Task.FrontMatter.MCPServers) | ||
| servers = append(servers, rule.FrontMatter.MCPServer) | ||
| } |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MCPServers method unconditionally appends the MCPServer from every rule, including empty/zero-value configs. This means if a rule doesn't specify an MCP server, an empty MCPServerConfig struct will be added to the returned slice. This could lead to invalid or meaningless entries in the results.
Consider filtering out empty MCP server configurations by checking if the config has meaningful values (e.g., non-empty Command or URL) before appending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in commit b1f3abb. MCPServers() now filters out empty configs by checking if Command or URL is set. Updated tests and documentation accordingly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Filter out empty MCP server configs in MCPServers() method - Only return configs with Command or URL set - Update tests to expect empty configs to be filtered - Remove duplicate standard fields list from example doc - Add documentation that empty configs are filtered Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.